fix: distinguish HTML entities from literal characters in diff comparison #7965
Conversation
…ison - Store original search/replace content before unescaping markers - Compare original content to preserve HTML entity distinction - Add comprehensive tests for HTML entity handling - Fixes issue where < and < were incorrectly treated as identical Fixes #7964
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| let startLine = replacement.startLine + (replacement.startLine === 0 ? 0 : delta) | ||
|
|
||
| // Store original content for comparison before any transformations | ||
| const originalSearchContent = searchContent |
There was a problem hiding this comment.
Is this approach intentional? We're storing both original and transformed content for every replacement, which doubles memory usage temporarily. For files with many replacements, could this become a performance concern, or is the trade-off acceptable for correctness?
|
|
||
| // Store original content for comparison before any transformations | ||
| const originalSearchContent = searchContent | ||
| const originalReplaceContent = replaceContent |
There was a problem hiding this comment.
Could we make this comment more specific? Something like:
| const originalReplaceContent = replaceContent | |
| // Store original content to preserve HTML entity distinction for identity comparison | |
| const originalSearchContent = searchContent | |
| const originalReplaceContent = replaceContent |
This would clarify why we need the original values.
| strategy = new MultiSearchReplaceDiffStrategy() | ||
| }) | ||
|
|
||
| it("should distinguish between HTML entities and their literal characters", async () => { |
There was a problem hiding this comment.
Great test coverage! Have we considered adding a test for nested/double-encoded HTML entities like < (which represents <)? This edge case could help ensure robustness with malformed or multiply-escaped content.
| } | ||
| }) | ||
|
|
||
| it("should handle the exact issue from bug report", async () => { |
There was a problem hiding this comment.
Could we add one more test case for mixed escaped and unescaped content in the same diff block? For example, a file that has some lines with < and others with < to ensure the comparison handles mixed scenarios correctly.
hannesrudolph
added
the
Issue/PR - Triage
|
Closing this PR as issue #7964 has been identified as a duplicate of #4077. The HTML entity handling during diff operations is a known issue that's being tracked in #4077. There's a proposed solution to add a user setting that would allow control over HTML entity escaping behavior. See #7964 (comment) for more details. |
Summary
This PR fixes issue #7964 where the diff tool incorrectly treated HTML entities (like
<and>) as identical to their literal characters (<and>), preventing valid replacements.Problem
When applying diffs to files, the tool was normalizing HTML entities too early in the comparison process, causing it to reject valid diffs that replaced escaped generic type syntax (
<...>) with real generic type syntax (<...>).Solution
The fix stores the original search/replace content before any transformations (unescaping markers) and uses this original content for the identity comparison. This preserves the distinction between HTML entities and their literal characters while still allowing the rest of the diff logic to work with normalized content.
Changes
multi-search-replace.tsto store and compare original contentmulti-file-search-replace.tswith the same fix for consistencyTesting
Review Confidence
Code review completed with 95% confidence score - implementation is sound and ready for merge.
Fixes #7964
Important
Fixes HTML entity handling in diff comparisons by storing original content for identity checks in
multi-search-replace.tsandmulti-file-search-replace.ts.multi-search-replace.tsandmulti-file-search-replace.ts.html-entity-handling.spec.tswith 7 test cases covering various HTML entity scenarios.applyDiff()in bothmulti-search-replace.tsandmulti-file-search-replace.tsto use original content for comparison.This description was created by
for 73de887. You can customize this summary. It will automatically update as commits are pushed.